Skip to content

Implements Type in SignalRValueProvider #116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Implements Type in SignalRValueProvider #116

wants to merge 3 commits into from

Conversation

danielwertheim
Copy link
Contributor

Implements the Type property in the SignalRValueProvider as it currently was left unassigned, or was this by design? Also introduces a specific SignalRNullValueProvider and unifies the creation of it using a simple factory method.

Didn't add the type checks that e.g. the ObjectValueProvider does as the factory method ensures the type is correct.

Also introduces a specific NullValueProvider
@wanlwanl
Copy link
Member

Thanks for contributing! The PR look good! I found NullObjectTask and SignalRNullValueProvider aren't frequently used, how about merge them into SignalRValueProvider so that we only keep one class?

@danielwertheim
Copy link
Contributor Author

Do you mean as nested classes or to move all logic into SignalRValueProvider?


public Type Type { get; }

public Task<object> GetValueAsync()
=> Task.FromResult(value);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wanlwanl is this a "hot" path? Would it be better to create a field holding a Task.FromResult(value) and set in in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the change I meant. If no good, I can revert.

@danielwertheim
Copy link
Contributor Author

@wanlwanl Merged the code into the SignalRValueProvider class so that the NullProvider isn't exposed anywhere else. I hope that was what you meant.

@wanlwanl
Copy link
Member

wanlwanl commented Mar 26, 2020

Hi @danielwertheim , I create a PR based on this PR, please take a look.

@danielwertheim
Copy link
Contributor Author

danielwertheim commented Mar 28, 2020

@wanlwanl I've changed so that the optimized null value provider etc. is enclosed in the SignalRValueProvider. I've also fixed that the actual type of the value is used. I've looked at other Azure projects and according to the purpose of ToInvokeString it seem to be to provide a string representation of the enclosed value. I'm a bit unsure about the generation of the string used in your PR suggestion. Wouldn't that suggestion be a breaking change as previously it was the ToString value of the contained value? Which seem to be the behavior of e.g. ObjectValueProvider in WebJobs project and in e.g. MicrosoftGraph extension project. So should that changes really be in this PR? I noticed it seemed not implemented in full as well as there were a todo added.

@wanlwanl
Copy link
Member

@danielwertheim Thanks for update!

SignalR extension used to add binding via BindToInput which uses BindToInputBindingProvider. I think we can refer the implementation of BindToInputBindingProvider.

As for ToInvoke, I also found some repos/blogs make it as a ToString, but in BindingBase, it mentions about "Called when we invoke from dashboard. ", I've asked an engineer in Function team, he told me "This is the WebJobs dashboard available in Kudu. This is primarily for WebJobs tests/direct invoke scenarios and doesn't apply to Functions much", so we don't need to worry about this method. I think keep it the same to BindingBase is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants